Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add custom rotation library, removing params and special cases in compass, ahrs and INS #19164

Merged
merged 14 commits into from
Mar 2, 2022
Merged

Add custom rotation library, removing params and special cases in compass, ahrs and INS #19164

merged 14 commits into from
Mar 2, 2022

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Nov 5, 2021

This removes the params and special cases for custom rotations. Instead adding support to AP math. If new rotations are selected the corresponding roll pitch and yaw params will show up.

This also means that custom rotations work with everything, and it is trivial to add more.

This adds two, so you could do a custom rotation on the AHRS and one compass, or two compass.

This also does param conversion.

@IamPete1 IamPete1 force-pushed the custom_rotations branch 2 times, most recently from 73e462f to 625b442 Compare November 5, 2021 18:13
@IamPete1
Copy link
Member Author

IamPete1 commented Nov 5, 2021 •
edited
Loading

Adds rather more than I had hoped.

Binary           text             data         bss            total
---------------  ---------------  -----------  -------------  ---------------
arduplane        1636 (+0.1688%)  0 (0.0000%)  4 (+0.0031%)   1640 (+0.1491%)
antennatracker   1640 (+0.2504%)  0 (0.0000%)  0 (0.0000%)    1640 (+0.2086%)
arducopter-heli  1668 (+0.1815%)  0 (0.0000%)  4 (+0.0031%)   1672 (+0.1592%)
arducopter       1692 (+0.1812%)  0 (0.0000%)  4 (+0.0031%)   1696 (+0.1593%)
ardurover        1740 (+0.2090%)  0 (0.0000%)  4 (+0.0031%)   1744 (+0.1809%)
blimp            1648 (+0.2445%)  0 (0.0000%)  0 (0.0000%)    1648 (+0.2047%)
ardusub          1636 (+0.2037%)  0 (0.0000%)  -4 (-0.0031%)  1632 (+0.1747%)

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 5, 2021

param conversion needs testing.

andyp1per
andyp1per approved these changes Nov 6, 2021
View reviewed changes
Copy link
Collaborator

@andyp1per andyp1per left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!
I'm surprised it adds so much. What about for only one custom rotation?

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 8, 2021

Marked for dev call to gauge where or not this is worth the flash cost.

There still seems to be some unknown issue with compass custom rotations that I was hoping this would resolve. It seems that a custom rotation is not the same as its equivalent hard-coded one.

@tridge
Copy link
Contributor

tridge commented Nov 8, 2021

don't do double precision for this

@IamPete1
Copy link
Member Author

IamPete1 commented Nov 9, 2021

Much more reasonable flash cost now its only storing single precision rotations.

Binary           text            data         bss           total
---------------  --------------  -----------  ------------  --------------
arduplane        240 (+0.0247%)  0 (0.0000%)  0 (0.0000%)   240 (+0.0218%)
antennatracker   244 (+0.0372%)  0 (0.0000%)  4 (+0.0031%)  248 (+0.0315%)
arducopter-heli  272 (+0.0296%)  0 (0.0000%)  0 (0.0000%)   272 (+0.0259%)
arducopter       296 (+0.0317%)  0 (0.0000%)  0 (0.0000%)   296 (+0.0278%)
ardurover        344 (+0.0413%)  0 (0.0000%)  0 (0.0000%)   344 (+0.0357%)
blimp            252 (+0.0374%)  0 (0.0000%)  4 (+0.0031%)  256 (+0.0318%)
ardusub          240 (+0.0299%)  0 (0.0000%)  0 (0.0000%)   240 (+0.0257%)

@IamPete1 IamPete1 force-pushed the custom_rotations branch 5 times, most recently from fdd1c64 to ce2b161 Compare November 13, 2021 02:10
@IamPete1
Copy link
Member Author

Added testing to the math rotations example.

peterbarker
peterbarker reviewed Nov 22, 2021
View reviewed changes
libraries/AP_CustomRotations/AP_CustomRotations_params.cpp
#include "AP_CustomRotations.h"
#include <AP_Vehicle/AP_Vehicle_Type.h>

#if !APM_BUILD_TYPE(APM_BUILD_AP_Periph)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have an AP_CUSTOMROTATIONS_ENABLED so that if a Periph really did want to allow custom rotations they could.

tridge
tridge requested changes Nov 23, 2021
View reviewed changes
libraries/AP_AHRS/AP_AHRS.cpp Outdated Show resolved Hide resolved
libraries/AP_Compass/AP_Compass.cpp Outdated Show resolved Hide resolved
libraries/AP_CustomRotations/AP_CustomRotations.cpp Outdated Show resolved Hide resolved
@Hwurzburg Hwurzburg added the WikiNeeded needs wiki update label Nov 23, 2021
@IamPete1 IamPete1 mentioned this pull request Nov 25, 2021
Open
@IamPete1 IamPete1 force-pushed the custom_rotations branch 4 times, most recently from 737987e to 392862e Compare February 23, 2022 13:48
@IamPete1 IamPete1 requested a review from tridge February 23, 2022 13:49
tridge
tridge previously requested changes Feb 28, 2022
View reviewed changes
libraries/AP_Compass/AP_Compass.cpp Outdated Show resolved Hide resolved
@IamPete1
Copy link
Member Author

IamPete1 commented Mar 1, 2022

Added notes on euler ordering and updated all of the rotation param descriptions to be the same. Also added a init allocation error.

@IamPete1
Copy link
Member Author

IamPete1 commented Mar 1, 2022

MatekF405:

Binary Name      Text [B]        Data [B]     BSS (B)       Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  --------------  -----------  ------------  ----------------------------  -------------------------
antennatracker   552 (+0.0836%)  0 (0.0000%)  0 (0.0000%)   552 (+0.0835%)                                   [32]
blimp            584 (+0.0860%)  0 (0.0000%)  0 (0.0000%)   584 (+0.0859%)                                   302928
ardurover        540 (+0.0644%)  0 (0.0000%)  4 (+0.0031%)  540 (+0.0644%)                                   14[33]
arducopter-heli  512 (+0.0545%)  0 (0.0000%)  0 (0.0000%)   512 (+0.0544%)                                    41692
ardusub          480 (+0.0593%)  0 (0.0000%)  0 (0.0000%)   480 (+0.0592%)                                   172424
arduplane        408 (+0.0424%)  0 (0.0000%)  0 (0.0000%)   408 (+0.0423%)                                    17264
arducopter       540 (+0.0583%)  0 (0.0000%)  4 (+0.0031%)  540 (+0.0582%)                                    54724

Periph f103-GPS:

Binary Name    Text [B]         Data [B]     BSS (B)      Total Flash Change [B] (%)      Flash Free After PR (B)
-------------  ---------------  -----------  -----------  ----------------------------  -------------------------
ap_periph      -160 (-0.1573%)  0 (0.0000%)  0 (0.0000%)  -160 (-0.1571%)                                    3752

@IamPete1 IamPete1 requested a review from tridge March 1, 2022 15:15
@tridge tridge merged commit 6ac661c into ArduPilot:master Mar 2, 2022
@IamPete1 IamPete1 removed the DevCallEU label Mar 9, 2022
@Hwurzburg Hwurzburg removed the WikiNeeded needs wiki update label Apr 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterbarker peterbarker peterbarker left review comments

@tridge tridge tridge approved these changes

@andyp1per andyp1per andyp1per approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@IamPete1 @tridge @andyp1per @peterbarker @CraigElder @Hwurzburg